Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automated Testing: Remove Puppeteer CI Job #59311

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

Mamaduka
Copy link
Member

What?

Now that Playwright migration is completed, we can safely remove Puppeteer and related.

Testing Instructions

CI should be green.

@Mamaduka Mamaduka added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label Feb 23, 2024
@Mamaduka Mamaduka self-assigned this Feb 23, 2024
@Mamaduka
Copy link
Member Author

I think this needs to become a peer dependency just like Playwright. @swissspidy, @gziolo what do you think?

"puppeteer-core": "^13.2.0",

Copy link

github-actions bot commented Feb 23, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: ellatrix <ellatrix@git.wordpress.org>
Co-authored-by: talldan <talldanwp@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org>
Co-authored-by: kevin940726 <kevin940726@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@Mamaduka Mamaduka force-pushed the remove/puppeteer-dep-and-ci branch from e76059c to 1ff8817 Compare February 23, 2024 11:44
@gziolo
Copy link
Member

gziolo commented Feb 23, 2024

I think this needs to become a peer dependency just like Playwright. @swissspidy, @gziolo what do you think?

"puppeteer-core": "^13.2.0",

As long as it works when using @wordpress/scripts alone which it should, it's fine with me.

@youknowriad
Copy link
Contributor

@Mamaduka can we remove the npm dependency entirely since we now recommend playwright, It can be a breaking change in the wp-scripts package but that seems fine.

Copy link
Member

@ellatrix ellatrix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this all that needs to be removed? What about the e2e-tests package?

@Mamaduka
Copy link
Member Author

Is this all that needs to be removed? What about the e2e-tests package?

First we’ll need to relocate test plugins in different directory and then I guess deprecate the package. Not sure what’s the policy about that.

@talldan
Copy link
Contributor

talldan commented Feb 26, 2024

First we’ll need to relocate test plugins in different directory and then I guess deprecate the package. Not sure what’s the policy about that.

I think @wordpress/nux is a package that has been deprecated in the past. There was also an attempt to remove it in the past which didn't go so well as it's used by core. This one might be different as it's only a dev dependency. 🤔

@ntsekouras
Copy link
Contributor

@Mamaduka can we remove the npm dependency entirely since we now recommend playwright, It can be a breaking change in the wp-scripts package but that seems fine.

➕ to this.

@kevin940726
Copy link
Member

Another smaller thing is that we can now reference npm run test:e2e directly to npm run test:e2e:playwright in this repo. We can keep the test:e2e:playwright script for a while for muscle memory.

Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed we're still running the puppeteer CI job. It takes five minutes even though it runs no tests 😆

Would love to see this PR merged to reduce our computations. ❤️

A small tracking issue for all the tasks associated with removing puppeteer would be great if anyone wants to volunteer to do that.

@talldan talldan force-pushed the remove/puppeteer-dep-and-ci branch from 1ff8817 to 34b004e Compare April 2, 2024 07:47
@talldan talldan enabled auto-merge (squash) April 2, 2024 07:48
@Mamaduka
Copy link
Member Author

Mamaduka commented Apr 2, 2024

Good point, @talldan!

I'll create a separate issue to remove Puppeteer from the scripts package. We can discuss the best options there.

@talldan talldan changed the title Project: Remove Puppeteer Automated Testing: Remove Puppeteer CI Job Apr 2, 2024
@talldan
Copy link
Contributor

talldan commented Apr 2, 2024

Decided to rebase this and it'll auto-merge when all tests pass.

Copy link

github-actions bot commented Apr 2, 2024

Flaky tests detected in 34b004e.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8519245299
📝 Reported issues:

@talldan talldan merged commit 1da3209 into trunk Apr 2, 2024
55 checks passed
@talldan talldan deleted the remove/puppeteer-dep-and-ci branch April 2, 2024 08:20
@github-actions github-actions bot added this to the Gutenberg 18.1 milestone Apr 2, 2024
@talldan
Copy link
Contributor

talldan commented Apr 2, 2024

Oops, I forgot to add the co-authors. Sorry everyone 😞

cbravobernal pushed a commit to garridinsi/gutenberg that referenced this pull request Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants